-
Notifications
You must be signed in to change notification settings - Fork 13.5k
Implement unstable trait impl #140399
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Implement unstable trait impl #140399
Conversation
d01f12a
to
9e139e1
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
486d3d3
to
02f780f
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
567ec89
to
80fd239
Compare
☔ The latest upstream changes (presumably #142299) made this pull request unmergeable. Please resolve the merge conflicts. |
850e3e1
to
27d9e21
Compare
Co-authored-by: lcnr <[email protected]>
|
||
let mut err = self.dcx().struct_span_err( | ||
span, | ||
format!("unstable feature `{sym}` is used without being enabled."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would try to mirror the existing errors for unstable library features if staged_api
is not enabled
error[E0658]: use of unstable library feature `slice_partition_dedup`
--> src/main.rs:3:7
|
3 | x.partition_dedup();
| ^^^^^^^^^^^^^^^
|
= note: see issue #54279 <https://github.com/rust-lang/rust/issues/54279> for more information
= help: add `#![feature(slice_partition_dedup)]` to the crate attributes to enable
= note: this compiler was built on 2025-07-02; consider upgrading it if it is out of date
not sure whehter you could even reuse the reporting machinery we already use for methods etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This diagnostics can be reported through feature_err_issue
, but it requires the issue number of the library feature. Unfortunately we don't have issue number information when we emit ambiguity error, and putting issue number into #[unstable_feature_bound]
can be quite verbose because there can be multiple symbol at a time, for example: #[unstable_feature_bound(foo, bar)]
So I added feature_err_unstable_feature_bound
in cf4770d, which is basically feature_err_issue
but omitted the issue number note.
tests/ui/unstable-feature_bound/unstable-feature-bound-two-error.rs
Outdated
Show resolved
Hide resolved
tests/ui/unstable-feature_bound/unstable-feature-cross-crate-multiple-symbol.rs
Outdated
Show resolved
Hide resolved
tests/ui/unstable-feature_bound/unstable_impl_coherence_inherence.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few more nits
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
#[stable(feature = "a", since = "1.1.1")] | ||
#[unstable_feature_bound(feat_bar)] | ||
fn bar() { | ||
Bar::foo(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not error in current implementation, we probably should lint against this usage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in, have an #[unstable_feature_bound(feat_bar)]
on a stable item?
yeah. Please add a fixme for this somewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, using anything annotated with #[unstable_feature_bound]
will need to be enabled through #[feature]
, so it will no longer be stable.
added a lint in 9cb97c2
@@ -20,6 +20,7 @@ rustc_parse_format = { path = "../rustc_parse_format" } | |||
rustc_session = { path = "../rustc_session" } | |||
rustc_span = { path = "../rustc_span" } | |||
rustc_transmute = { path = "../rustc_transmute", features = ["rustc"] } | |||
rustc_type_ir = {path = "../rustc_type_ir"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please go through rustc_middle
instead, it should reexport all of rustc_type_ir
in ty
#[allow(rustc::usage_of_type_ir_traits)] | ||
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | ||
return ProcessResult::Changed(Default::default()); | ||
} else { | ||
return ProcessResult::Unchanged; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[allow(rustc::usage_of_type_ir_traits)] | |
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | |
return ProcessResult::Changed(Default::default()); | |
} else { | |
return ProcessResult::Unchanged; | |
} | |
if self.selcx.infcx.may_use_unstable_feature(obligation.param_env, symbol) { | |
ProcessResult::Changed(Default::default()); | |
} else { | |
ProcessResult::Unchanged; | |
} |
cc @compiler-errors I don't think we should use the InferCtxtLike
trait here. However, I can't think of a good place to put this.
We could make it a free function and use the reexport from rustc_middle
, but 🤷 idk, also feels kinda iffy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya, I don't see why it shouldn't just be a free function. Why does it use InferCtxtLike
today?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I basically need a place to put may_use_unstable_feature
that is accessible by old and new solvers, ideally without too much change, and InferCtxtLike
just happened to work in a7eada0. But yeah, free function should work too.
#[stable(feature = "a", since = "1.1.1")] | ||
#[unstable_feature_bound(feat_bar)] | ||
fn bar() { | ||
Bar::foo(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as in, have an #[unstable_feature_bound(feat_bar)]
on a stable item?
yeah. Please add a fixme for this somewhere
@@ -0,0 +1,15 @@ | |||
//@ aux-build:unstable_impl_coherence_inference_aux.rs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this file is called tests/ui/unstable-feature_bound/unstable_impl_coherence_inherence.rs
rn, not inference
. and why call it coherence
🤔 this is more about method selection, is it not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah method selection is a better name, fixed :>
Could you please do a perf run before this lands too? The extra call to |
Sure, I will do a rebase after lcnr finish reviewing, then request for a perf run. |
This PR allows marking impls of stable trait with stable type as unstable.
Approach
In std/core, an impl can be marked as unstable by annotating it with
#[unstable_feature_bound(feat_name)]
. This will add aClauseKind::Unstable_Feature(feat_name)
to the list of predicates inpredicates_of
.When an unstable impl's function is called, we will first iterate through all the goals in
param_env
to check if there is anyClauseKind::UnstableFeature(feat_name)
inparam_env
.The existence of
ClauseKind::Unstable_Feature(feat_name)
inparam_env
means an#[unstable_feature_bound(feat_name)]
is present at the call site of the function, so we allow the check to succeed in this case.If
ClauseKind::UnstableFeature(feat_name)
does not exist inparam_env
, we will still allow the check to succeed for either of the cases below:#[feature(feat_name)]
outside of std / core.For the rest of the case, it will fail with ambiguity.
Limitation
In this PR, we do not support:
#[unstable_feature_bound]
within stable APIs#[unstable_feature_bound]
#[unstable_feature_bound]
on items other than free function and implAcknowledgement
The design and mentoring are done by @BoxyUwU